Elimination of the PickleIndex from Row Test Method Signature #938

clrudolphi · 2025-11-22T23:15:36Z

🤔 What's changed?

This is how we eliminate the pickleIndex parameter from the signature of generated row tests.

A hash is taken of FeatureName, ScenarioName, Example tags, and row values. This hash is inserted as a hidden tag on the pickle that is generated at compile time. These are stored as resources in the test assembly.

Generated test method is changed to calculate this hash at run-time, look for a matching pickle and use its index from the list of pickles. If a test has duplicate rows (and with the same example tags) we can't predict which matching pickle will get assigned at runtime, but I think that is acceptable as there is no discernable difference between these tests anyway.

Retries still work as pickle indices are assigned for a given row hash on a round-robin basis and can be repeated.

⚡️ What's your motivation?

Removes an internal implementation detail from being visible to test frameworks and other CI tools.
This addresses the concerns raised in issue #927

🏷️ What kind of change is this?

💥 Breaking change (incompatible changes to the API)

♻️ Anything particular you want feedback on?

This prototype uses an MD5 hash of a string (which is a concatenation of Feature Name, Scenario Name, Example Tags, and Example Row table values). The hash is stored in as a tag on the pickle. Is a hash really required here? We could simply store the concatenated string as the value of the tag. (End users never see this tag)
If we do wish to use a hash function, is MD5 acceptable?
We had previously discussed on other channels to include in the hash the Example set name and/or index. I can't think of a way of obtaining that information at run-time. Let me know if I'm missing something.
All the hash handling logic is located in a new static class in Reqnroll (and referenced by the Reqnroll.Generator). Should this be a dependency injected class instead?
Is the logic used to match a running row test with a pickle acceptable (from a thread safety perspective)?

📋 Checklist:

I've changed the behaviour of the code
- I have added/updated tests to cover my changes.
My change requires a change to the documentation.
- I have updated the documentation accordingly.
Users should know about my change
- I have added an entry to the "[vNext]" section of the CHANGELOG, linking to this pull request & included my GitHub handle to the release contributors list.

This text was originally taken from the template of the Cucumber project, then edited by hand. You can modify the template here.

clrudolphi · 2025-11-22T23:16:31Z

This works on simple example. CI failing; will look into test failures tomorrow.

obligaron · 2025-11-23T10:27:28Z

Reqnroll/Formatters/RuntimeSupport/TestRowPickleMapper.cs

+            lock (pickle.Tags)
+            {
+                var tag = pickle.Tags.FirstOrDefault(t => t.Name == tagName);
+                if (tag != null)
+                {
+                    pickle.Tags.Remove(tag);
+                    return i.ToString();
+                }
+            }


Does this work with retries?

I'm not following; what type of retry are you considering as a potential problem?

If a test runner decides to re-run the outline example test within the same execution process, the second run will not find the pickle index.

obligaron · 2025-11-23T10:28:35Z

Reqnroll/Formatters/RuntimeSupport/TestRowPickleMapper.cs

+        var tagsList = tags ?? Enumerable.Empty<string>();
+        var rowValuesList = rowValues ?? Enumerable.Empty<string>();
+        var v = $"{featureName}|{scenarioOutlineName}|{string.Join("|", tagsList)}|{string.Join("|", rowValuesList)}";
+        return v.GetHashCode();


HashCodes aren't likely to overlap but they are not unique.
Is this a problem here?

I've changed this is an MD5 hash.

clrudolphi · 2025-11-24T21:53:09Z

The CI build and tests are passing on github.
But I do have the weirdest set of failures on my local that I can't figure out.
The Reqnroll.SystemTests.Portability test suite has failures for the same test across .Net462, 472, and 481 for the Reqnroll.SystemTests.Portability.NetxxxPortabilityTest.GeneratorAllIn_sample_can_be_handled test ONLY when run using TUnit.
These tests fail because no TRX file is created.
The test log includes this:
error : Testing with VSTest target is no longer supported by Microsoft.Testing.Platform on .NET 10 SDK and later. If you use dotnet test, you should opt-in to the new dotnet test experience.
But why wouldn't other similar tests also fail?

Has anyone come across something similar?

gasparnagy

I have done a first round of review, here are my comments.

I still need to think of the re-run problem that was mentioned here, but I will post about that separately.

Reqnroll/Formatters/ExecutionTracking/FeatureExecutionTracker.cs

Reqnroll/Formatters/RuntimeSupport/TestRowPickleMapper.cs

gasparnagy · 2025-11-26T10:33:59Z

Reqnroll/Formatters/RuntimeSupport/TestRowPickleMapper.cs

+            lock (pickle.Tags)
+            {
+                var tag = pickle.Tags.FirstOrDefault(t => t.Name == tagName);
+                if (tag != null)
+                {
+                    pickle.Tags.Remove(tag);
+                    return i.ToString();
+                }
+            }


If a test runner decides to re-run the outline example test within the same execution process, the second run will not find the pickle index.

gasparnagy · 2025-11-26T10:35:39Z

Reqnroll/FeatureInfo.cs

        /// This property holds the cucumber messages at the feature level created by the test class generator; populated when the FeatureStartedEvent is fired. Used internally.
        /// </summary>
-        internal IFeatureLevelCucumberMessages FeatureCucumberMessages { get; set; }
+        public IFeatureLevelCucumberMessages FeatureCucumberMessages { get; set; }


Do we have to make this public? It would be better if feature info would only expose things publicly that the users can/should work with.

I see it now - it is used in the generated code. I would pass the entire FeatureInfo to GetPickleIndexFromTestRow, so that it can access the internal prop.

reverted and modified code gen per suggestion.

…romRowTestMethodSignature

gasparnagy · 2025-11-26T11:13:39Z

OK. I have an idea how to make this more robust and also support re-runs.

In FeatureLevelCucumberMessages class, when you initialize the _pickles field (here), once the pickles are loaded, you should immediately process them: take out and remove the hash tag. The type field _pickles field needs to be changed to Lazy<List<(Pickle Pickle,string Hash)>> to be able to "remember" the hash. So we will know which hash belongs to which pickle.
Add a Concurrent Bag to FeatureLevelCucumberMessages to be able to track which pickle (or pickle index) has been "used" already.
Add a method to FeatureLevelCucumberMessages class that acquires a pickle index by hash:
a. it tries to find a pickle with the has that is not yet in the concurrent bag: if it founds, it adds it to the concurrent bag and returns its index. (This is the happy path)
b. if it cannot find such, it should just find one that has the hash, but do not check the concurrent bag. This will support re-runs: if a test is being re-runned the re-run will always be attached the the first of the duplicated prickles - this is an OK compromise.
c. if there is no pickle with the provided hash, it should simply return the first pickle index - this is just for safety
The TestRowPickleMapper.GetPickleIndexFromTestRow can just call this acquire method after calculating the hash

@clrudolphi does that make sense?

clrudolphi · 2025-11-26T16:47:08Z

I have an idea how to make this more robust and also support re-runs.

I had not yet given retry semantics much thought for this PR yet. Now that I think about it, it will be a mess and not much that we can do about it.
The timing of when a test framework may issue a retry of a pickle is out of our control. So if we have two example table rows that are identical (say R1 and R2), the sequence of execution might be:

R1, R2 : assigned to P1, P2 respectively
R2, R1 : assigned to P1, P2 (not strictly correct but there is no way for anyone to know the difference)
R1, R2, R1(retry) : assigned to P1, P2, and P1 (as we re-use from the top)
R1, R1(retry), R2 : assigned to P1, P2, and P1 (which confuses the successful completion of the two pickles)
R2, R1, R1(retry) : assigned to P1, P2, and P1 (again confusion)

Using a first-come, first-served approach to assigning PickleIDs to rows will result in unintuitive results in the latter two situations.

I'll think about this a bit more.

gasparnagy · 2025-11-26T17:16:34Z

Using a first-come, first-served approach to assigning PickleIDs to rows will result in unintuitive results in the latter two situations.

I'll think about this a bit more.

Sure, but please note that this only happens if the two examples are fully identical and you use such retry functionality. In that case you could anyway consider that the two examples are retries of each-other. I'm not saying that the behavior is correct, but the situation is so special, that the behavior can be "good enough".

…ing the code gen to pass the FeatureInfo to the test row mapper.

…k which pickleIndex is given out to an executing test. TestRowPickleMapper is simplified and limited to hash computation and matching/removing the PickleTag from the pickle. Tracking of which pickleIndices have been used is movedto FeatureLevelCucumberMessages. Marker PickleTags are removed at startup (FeatureLevelCucumberMessages constructor). PickleIndices are assigned on a round-robin basis for a given rowHash.

clrudolphi · 2025-11-26T21:43:54Z

Thanks for the suggestions and reassurance.
I've adopted your suggestion of processing the tags upon startup in the FeatureLevelCucumberMessages constructor.
The tracking now uses a round-robin approach. For each rowHash we keep a list of pickleIndex values and an index into that list. As each pickleIndex is assigned to a test execution, the index is incremented modulo the count; thus wrapping around to start the reassignment all over with the next execution.

…aving an "internal" public method on FeatureInfo

gasparnagy

I think this is good now, but since this is a kind of breaking change (as we have seen), I would feel better to put this to v4. What do you think?

clrudolphi · 2025-12-17T00:40:54Z

I'll take a final look at the last set of changes.

But, yes, agreed this should wait until v4.

StarWars999123 · 2025-12-17T00:48:18Z

I will try to take a look at a build and gather some experiences with it after X-mas. I am totally fine with v4. We rolled back to v2 and can continue work with this so far.
When is v4 gonna be roughly scheduled for release? like mid 26 or later? (I don't know where to find such a timeline right now)

Anyway Thank you a lot!

gasparnagy · 2025-12-17T06:40:50Z

@StarWars999123 I hope to have v4 in early 2026, but definitely in Q1.

gasparnagy · 2025-12-17T06:44:31Z

I'll take a final look at the last set of changes.

Yes, sure. I wanted to highlight anyway that I've made a bit of refactoring that you should double-check. I just forgot... 😎

…pe of the code that is being generated. Added Changelog entry.

clrudolphi · 2025-12-17T16:43:38Z

I think this is ready for v4.
Although, I will admit, that the code we added here is dense and unintuitive. I'm worried about maintainability. I struggled to understand it after being away from it for just a few weeks.

gasparnagy · 2025-12-17T17:10:46Z

I think this is ready for v4. Although, I will admit, that the code we added here is dense and unintuitive. I'm worried about maintainability. I struggled to understand it after being away from it for just a few weeks.

I had a bit of the same feeling. But the good news is, that essentially we don't need the pickle index concept. I have made a prototype that simply uses the pickle ID everywhere. That makes the whole thing much more straightforward. I have dropped it finally, because there is a disadvantage: every time we generate the code-behind, a new ID will be included. But once people will move on the let the code-behind generated in the obj folder, no one will care and also the new up-to-date checking infra we have is not sensitive for this anymore. We just need to wait a bit until these things get a bit more mature.

And the massive amount of tests we have is a quite good protection anyway until then.

Prototype; initial commit.

78510ee

clrudolphi requested review from Code-Grump, gasparnagy and obligaron November 22, 2025 23:15

obligaron reviewed Nov 23, 2025

View reviewed changes

clrudolphi added 3 commits November 24, 2025 11:45

Fixes.

523982b

Adjust TUnit TestGeneratorProvider's DisplayName logic.

8fd8575

Minor cleanups

b4a1517

clrudolphi requested a review from obligaron November 24, 2025 21:44

gasparnagy mentioned this pull request Nov 25, 2025

Extra index parameter added to Scenario Outline Example methods in Reqnroll 3.x #927

Open

gasparnagy reviewed Nov 26, 2025

View reviewed changes

Merge remote-tracking branch 'origin/main' into EliminatePickleIndexF…

302ba97

…romRowTestMethodSignature

clrudolphi added 4 commits November 26, 2025 11:45

Made the row hash marker prefix a constant defined in a single location.

46a9eca

Reverted FeatureInfo's FeatureCucumberMessages to be internal; modify…

a536a37

…ing the code gen to pass the FeatureInfo to the test row mapper.

Merge branch 'main' into EliminatePickleIndexFromRowTestMethodSignature

83ceccd

clrudolphi marked this pull request as ready for review November 26, 2025 21:43

clrudolphi requested a review from gasparnagy November 26, 2025 21:46

gasparnagy added 5 commits December 16, 2025 14:10

Merge branch 'main' into EliminatePickleIndexFromRowTestMethodSignature

efa40d8

fix merge conflict resolution

22cb837

cosmetic fixes

ffb8329

refactor to calculate pickles and map together in a lazy fashion

b67bd67

Move back GetPickleIndexFromTestRow to TestRowPickleMapper to avoid h…

9956581

…aving an "internal" public method on FeatureInfo

gasparnagy approved these changes Dec 16, 2025

View reviewed changes

304NotModified added this to the v4 milestone Dec 17, 2025

clrudolphi changed the title ~~Prototype of Eliminating the PickleIndex from Row Test Method Signature~~ Elimination of the PickleIndex from Row Test Method Signature Dec 17, 2025

clrudolphi added 2 commits December 17, 2025 10:22

Merge branch 'main' into EliminatePickleIndexFromRowTestMethodSignature

444cdce

Cosmetic - updated comment in the method generator to reflect the sha…

9303148

…pe of the code that is being generated. Added Changelog entry.

clrudolphi linked an issue Jan 21, 2026 that may be closed by this pull request

Extra index parameter added to Scenario Outline Example methods in Reqnroll 3.x #927

Open

Elimination of the PickleIndex from Row Test Method Signature #938

Are you sure you want to change the base?

Elimination of the PickleIndex from Row Test Method Signature #938

Uh oh!

Conversation

clrudolphi commented Nov 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤔 What's changed?

⚡️ What's your motivation?

🏷️ What kind of change is this?

♻️ Anything particular you want feedback on?

📋 Checklist:

Uh oh!

clrudolphi commented Nov 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clrudolphi commented Nov 24, 2025

Uh oh!

gasparnagy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gasparnagy commented Nov 26, 2025

Uh oh!

clrudolphi commented Nov 26, 2025

Uh oh!

gasparnagy commented Nov 26, 2025

Uh oh!

clrudolphi commented Nov 26, 2025

Uh oh!

gasparnagy left a comment

Choose a reason for hiding this comment

Uh oh!

clrudolphi commented Dec 17, 2025

Uh oh!

StarWars999123 commented Dec 17, 2025

Uh oh!

gasparnagy commented Dec 17, 2025

Uh oh!

gasparnagy commented Dec 17, 2025

Uh oh!

clrudolphi commented Dec 17, 2025

Uh oh!

gasparnagy commented Dec 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

clrudolphi commented Nov 22, 2025 •

edited

Loading

clrudolphi commented Nov 22, 2025 •

edited

Loading